Issue #9646: Change Windows title bar based on theme

Merged Alx Sa requested to merge alxsa-titlebar-darkmode-win into master

This attempts to resolve issue #9646 (closed).

We can use DwmSetWindowAttribute () to set dialogue titlebars to either dark or light mode on Windows.

  • For the main window, this is done by checking the prefer-dark-theme GUI config property on start-up and when the theme is refreshed.
  • For other dialogues, this is done by checking the dialogue's background color and setting to dark mode if it's below a threshold (currently 0.5, 0.5, 0.5) since we don't have access to the GUI config property. I've added the function call to GimpDialog, GimpFileDialog, and GimpAboutDialog (since it seems to be its own thing) which so far handles most dialogues.

The main caveat is that this won't update already open dialogue title bars until they're reopened. Since the only way this would happen is if you open Preferences while doing other work and make the change, I don't think it's worth the additional complexity (at least until we can do this for all OS and not just Windows). 😄

@brunolopesdsilv: Hi! When you have time, could you test this and see what you think? I'm interested if there are any performance issues, graphical glitches, and if I've missed any dialogues (last one is an easy fix). Thanks!

@Jehan Hello! If you have time, can you give this a once-over and let me know if there are any structure issues or changes needed? I assume eventually we would want to use this same code for the MacOS title bars and maybe Linux (unless there's a better way already built into GTK), so let me know if I have wrong assumptions or need to re-arrange things. Thanks!

Merge request reports

Merge request pipeline #585913 failed

Merge request pipeline failed for ad8b47bf

Approval is optional

Merged by Alx SaAlx Sa 1 week ago (Oct 18, 2023 7:15pm GMT+0200)

Merge details

Pipeline #586122 passed

Pipeline passed for ad8b47bf on master

Activity

  • Jehan
    Jehan @Jehan started a thread on an old version of the diff
    Resolved by Alx Sa
    92 va_list args);
    93
    94 gint gimp_dialog_run (GimpDialog *dialog);
    95 95
    96 96 void gimp_dialog_set_alternative_button_order_from_array
    97 (GimpDialog *dialog,
    98 gint n_buttons,
    99 gint *order);
    97 (GimpDialog *dialog,
    98 gint n_buttons,
    99 gint *order);
    100 100
    101 GBytes * gimp_dialog_get_native_handle (GimpDialog *dialog);
    101 GBytes * gimp_dialog_get_native_handle (GimpDialog *dialog);
    102
    103 void gimp_dialog_set_title_bar_theme (GtkWidget *dialog);
    • Maintainer

      Hmmm… I don't get why make it a public function. It does seem to me like it is the obvious thing to do by default. Just make it a static function. If someone actually asks for it to be a public function and provides reasonable use case to support this request, we'll think if this is worth it.

    • Author Developer

      @Jehan Ah, okay. I was thinking that theme makers might want to override the title bar, but since the current code doesn't allow for that anyway it's a moot point. 😄

      However, GimpAboutDialog no longer builds if I make the function static. I guess I'd need to change it to inherit from GimpDialog rather than GtkDialog?

    • Maintainer

      Yes, that's because I see you use gimp_dialog_set_title_bar_theme in app/dialogs/about-dialog.c and app/widgets/gimpfiledialog.c.

      One solution could be to create a libgimpwidgets/gimpdialog-private.h, which would not be installed and put the function in there. It would still be exported (as long as you don't underscore the function name, which our build makes into an internal function), so it could be used by app/ code (as long as it's included there). Or simpler (though essentially the same thing): move it to libgimpwidgets/gimpwidgets-private.h.

      This being said, you basically already have the same code inside app (themes_set_title_bar()), and in fact an even better version as you don't have to guess if a theme is dark or not. You could just move this code into app/widgets/gimpwidgets-utils.[ch] with both Gimp * and GtkWidget * arguments. Inside app/gui/themes.c, only keep the code looping through windows and make it call the utils function. In the about and file dialogs, also call this utils function.

      This second solution seems better than the first because you already have the code in app/ and with more theme knowledge.

      I guess I'd need to change it to inherit from GimpDialog rather than GtkDialog?

      Sure, it could be a third version, though having an utils function which works on every type of windows would be good too anyway.

    • Alx Sa changed this line in version 2 of the diff ·

      changed this line in

    • Author Developer

      @Jehan Thanks! If I understood you correctly, my function in gimpwidgets-utils.c should handle both possibilities (e.g. if we have access to the GIMP object we can get the config property, and if we don't we fallback to checking the background color). I updated the code based on that idea.

    • Please register or sign in to reply
  • Maintainer

    Looks fine to me, except that we should likely not have any public function. Just makes this happen on all GimpDialog and done.

    This way, it is self-contained enough that it's not a problem if we need to update the logic (e.g. if we have a better solution than the "guessing dark mode" by background color) in the future or reorganize things.

  • Alx Sa added 1 commit

    added 1 commit

    • 22e1f9a9 - themes: Update structure per Jehan feedback

    Compare with previous version

  • Hi @cmyk.student . I'm still struggling to reconfigure my build setup, but I hope I'll be able to finally test it today.

    (CI Windows builds without the installer are just useless since the .zip artifact somewhat don't have icons and scheme files that lead GIMP to not run)

    Edited by Bruno Lopes
  • Jehan
    Jehan @Jehan started a thread on an old version of the diff
    Resolved by Alx Sa
    420 427 }
    421 428 }
    422 429
    430 static void
    431 gimp_file_dialog_map (GimpFileDialog *dialog,
    432 gpointer data)
    433 {
    434 #ifdef G_OS_WIN32
    435 gimp_window_set_title_bar_theme (NULL, GTK_WIDGET (dialog));
  • Jehan
    Jehan @Jehan started a thread on an old version of the diff
    Resolved by Alx Sa
    206 208
    207 209 dialog->timer = g_timeout_add (800, about_dialog_timer, dialog);
    208 210 }
    211
    212 #ifdef G_OS_WIN32
    213 gimp_window_set_title_bar_theme (NULL, widget);
    • Maintainer

      What about adding Gimp *gimp as arg to about_dialog_create()? I see it's used in 2 places:

      1. In app/dialogs/dialogs-constructors.c where we have context->gimp.
      2. In app/gimp-update.c where it would be extra-easy to make gimp object the user_data of gimp_update_about_dialog() (right now we set it as NULL).

      Basically within app/, we have access to this object. Let's just pass it through to get the correct info for sure, rather than guessing.

    • Alx Sa changed this line in version 3 of the diff ·

      changed this line in

    • Author Developer

      @Jehan Ah, I see. Okay, fixed as well.

    • Please register or sign in to reply
  • Jehan
    Jehan @Jehan started a thread on the diff
    Resolved by Alx Sa
    2627 void
    2628 gimp_window_set_title_bar_theme (Gimp *gimp,
    2629 GtkWidget *dialog)
    2630 {
    2631 #ifdef G_OS_WIN32
    2632 HWND hwnd;
    2633 GdkWindow *window = NULL;
    2634 GimpGuiConfig *config;
    2635 gboolean use_dark_mode = FALSE;
    2636
    2637 window = gtk_widget_get_window (GTK_WIDGET (dialog));
    2638 if (window)
    2639 {
    2640 if (gimp)
    2641 {
    2642 config = GIMP_GUI_CONFIG (gimp->config);
    • Maintainer

      Note: in fact, I notice that we don't really need gimp but GIMP_GUI_CONFIG (gimp->config). So instead of a Gimp * arg, you could set a GimpGuiConfig * arg. Anyway that's a detail. You could stick with Gimp *, it's fine too.

    • Please register or sign in to reply
  • Jehan
    Jehan @Jehan started a thread on the diff
    Resolved by Alx Sa
    750 769 {
    751 770 show_help_button = show ? TRUE : FALSE;
    752 771 }
    772
    773 void
    • Maintainer
      Resolved by Alx Sa

      If I understood you correctly, my function in gimpwidgets-utils.c should handle both possibilities (e.g. if we have access to the GIMP object we can get the config property, and if we don't we fallback to checking the background color).

      In fact, inside app, we always have access to the gimp object, at least so far in the pieces of code you touched (cf. my review). Maybe in some very specific cases, it would be more cumbersome to get the object (though probably never impossible). But in your current version of the patch, it's actually quite easy!

      I gave you instructions in my review for the 2 cases where you were passing NULL.

    • Alx Sa Jehan Bruno Lopes Last reply by Alx Sa
    • I was unable to compile either with the instructions on the developer's website or with the CI script. I feel very stupid, because I was able to do it both ways a few weeks ago.

      Two types of errors happens:

      1. The deps libs aren't copied to the bin folder
      2. Even if i take needed .dll's from a 2.99 install, other error, like the CI builds are displayed: lack of icons and some scheme files
    • Author Developer

      @brunolopesdsilv No worries! I need to make a further revision anyway based on Jehan's feedback, so maybe you'll be able to compile again before I finish.

    • The problem is that your code builds normally and something wrong happens in the "ninja install" phase. It's not your fault.

      I'll open an issue about this. More or less after the 2.9.18 milestone, it's only possible to use self-build GIMP on Windows after packaging.

    • Please register or sign in to reply
  • Alx Sa added 1 commit

    added 1 commit

    • aaf529ce - themes: Further revisions based on Jehan's feedback

    Compare with previous version

  • Alx Sa added 11 commits

    added 11 commits

    • aaf529ce...701357c0 - 8 commits from branch master
    • a996e916 - gui: Change Windows title bar based on theme
    • b0d6d581 - themes: Update structure per Jehan feedback
    • 6eec5174 - themes: Further revisions based on Jehan's feedback

    Compare with previous version

    Toggle commit list
  • Jehan
    Jehan @Jehan started a thread on an old version of the diff
    Resolved by Alx Sa
    52 54 {
    53 55 GtkWidget *dialog;
    54 56
    57 Gimp *gimp;
  • Alx Sa added 1 commit

    added 1 commit

    • e0255810 - gui: Change Windows title bar based on theme

    Compare with previous version

  • Maintainer

    For info, globally the code looks fine to me (though I haven't tested it). I trust you'll do whatever necessary tests, and feel free to merge. :-)

  • Alx Sa added 1 commit

    added 1 commit

    • e2997e52 - gui: Change Windows title bar based on theme

    Compare with previous version

  • Alx Sa added 3 commits

    added 3 commits

    Compare with previous version

  • Alx Sa enabled an automatic merge when the pipeline for ad8b47bf succeeds

    enabled an automatic merge when the pipeline for succeeds

  • Alx Sa canceled the automatic merge

    canceled the automatic merge

  • merged

  • Alx Sa changed milestone to %2.99.18

    changed milestone to

Please register or sign in to reply